Miscellaneous scrapping fixes - #378

Thank you for the PR! I kicked off the CI for completeness, but the failures do not necessarily indicate that something is amiss with your PR. I will run some tests locally overnight and report back.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion

bazel_ros2_rules/ros2/resources/cmake_tools/file_api.py line 72 at r1 (raw file):

                    self.link_flags.append(fragment['fragment'])
                elif fragment['role'] == 'libraries':
                    # In the generated JSON a library can be in quotes (to escape some characters?)

Nit: We try to wrap lines at 80 characters whenever possible. This applies to the other files in the PR as well.

I took another pass looking through the changes this evening. This looks quite good. I have a couple small questions and nits below. Thank you for iterating on this.

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gergondet-woven)

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/ament_python.py line 17 at r2 (raw file):

    top_level = dist.read_text('top_level.txt')
    top_level = top_level.rstrip('\n')
    return str(dist._path), "\n".join(

Nit: Consider storing this string in a variable and returning that for readability. Also, consider adding a comment as to what this is fixing.

bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 156 at r2 (raw file):

        sandbox(top_level_i)
        for _, top_level in properties.python_packages
        for top_level_i in top_level.split("\n")

Nit: This nested list comprehension is quite clever, but somewhat hard to understand at a glance. Consider breaking it up for readability.

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

                    elements_to_remove.append((parent, child))
                else:
                    child.attrib.pop("condition")

Nit: Consider conforming to the single quote string convention used by the rest of the file.

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

                    elements_to_remove.append((parent, child))
                else:
                    child.attrib.pop("condition")

Looking at the ElementTree iter documentation, it states that the behavior is undefined when iterating over a tree if it is then modified.

Would child.attrib.pop('condition') be considered a modification to the tree structure?

bazel_ros2_rules/ros2/resources/cmake_tools/file_api.py line 76 at r2 (raw file):

                    # This escaping creates problem down the line,
                    # "/usr/lib/libfoo.so" is *not* a path.
                    # We remove the escaping here so they can be treated normally

This comment block is fantastic for conveying the issue and fix. Thanks for this!

Thanks again for the feedback and sorry for the delay but I finally got around to fixing the issues you raised!

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @IanTheEngineer)

bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 156 at r2 (raw file):

Done, hopefully it's clearer

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/ament_python.py line 17 at r2 (raw file):

Done, I have split this into two assignments: one to retrieve the entries inside the the top_level file and one to resolve the path. I have also added a comment clarifying why we need to do this here

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

Sorry about that, I have changes all the double quotes in the patch with single quotes

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

I don't think so, attributes are attached to the node so removing them should not modify the tree structure.

Thank you for the iterations here! Things look great. I'll give this a test run tomorrow and we should be clear to merge.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)

I believe that we are de facto committed to a legacy support branch of some sort, either for jammy or humble; we should avoid if possible having two legacy support branches at once.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)

Reviewed all commit messages.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @IanTheEngineer)

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/python.py line 26 at r4 (raw file):

    if len(packages) > 1:
        print(f"Multiple top level entries where found in {name}. "
               "Only the first one will be considered")

BTW: If the only place this is called is line 53 of this file, you could just return the whole list of packages and not build a singleton list around it there?
(I doubt this is especially important, but it would also probably net simplify the code)

What's the current status on this PR? I see one open discussion and some unclear CI status; I kicked the CI to unwedge that. Who has the next action on this PR?

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @gergondet-woven and @IanTheEngineer)

Is this failure related?
https://github.com/RobotLocomotion/drake-ros/actions/runs/14969360103/job/42075274838?pr=378

Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @IanTheEngineer)

Reviewed 2 of 5 files at r2, 3 of 4 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)